ENH: enable TLS/SSL through local filepath in http source #359

chenqi0805 · 2021-10-05T16:24:00Z

Description

This PR

Refactors the otel-trace-source certificate model and provider into common plugin.
Add certificate provider factory in http source to reuse the refactored certificate providers
Add test coverage on https protocol in http source.

Note: For now we only support certificates from local file path corresponding to the config in Fluentbit HTTP plugin (client).

Issues Resolved

#358

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: qchea <[email protected]>

laneholloway

I've left a few comments, they aren't blockers but we should open up issues on them so that we don't lose sight of fixing them.

...ugins/common/src/main/java/com/amazon/dataprepper/plugins/certificate/model/Certificate.java

laneholloway · 2021-10-05T17:15:20Z

data-prepper-plugins/http-source/build.gradle

    implementation "com.linecorp.armeria:armeria:1.9.2"
+    implementation "commons-io:commons-io:2.11.0"


We ought to move the versions of dependencies we use in multiple places out to the external file with the others that we've already defined.

Right now we have a versionsMap in our build-resources.gradle to provide this consistency.

Gradle 7 should help us improve this, but it is not possible to update presently due to the OpenSearch plugin.

We could add commons-io version into versionMap. There might be other common packages. We should do that in a separate PR.

Please make a separate PR for this.

laneholloway · 2021-10-05T17:17:13Z

...gins/http-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSource.java

@@ -44,8 +51,19 @@ public void start(Buffer<Record<String>> buffer) {
        }
        if (server == null) {
            final ServerBuilder sb = Server.builder();
-            // TODO: allow tls/ssl
-            sb.http(sourceConfig.getPort());
+            if (sourceConfig.isSsl()) {


Nit: do we want to have isSsl or isSSL given that SSL is an acronym and not a proper word?

I think isSSL makes more sense. This getter is autogenerated.

I think we should stick with isSsl. Making initials lowercase is a common convention. For example, java.net.http.HttpRequest.

laneholloway · 2021-10-05T17:20:43Z

data-prepper-plugins/opensearch/build.gradle

        force 'commons-logging:commons-logging:1.2'
        force 'org.apache.httpcomponents:httpclient:4.5.13'
+        force 'org.apache.httpcomponents:httpcore:4.4.13'
        force "org.hdrhistogram:HdrHistogram:2.1.12"
        force 'joda-time:joda-time:2.10.10'


not for this PR, but we really shouldn't be using Joda time since it is now included in the JDKs.

This might be here because the OpenSearch build plugin brings in its own dependencies.

Ok, we can dive into it later.

...ttp-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceConfig.java

dlvenable · 2021-10-05T20:36:59Z

...gins/http-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSource.java

@@ -44,8 +51,19 @@ public void start(Buffer<Record<String>> buffer) {
        }
        if (server == null) {
            final ServerBuilder sb = Server.builder();
-            // TODO: allow tls/ssl
-            sb.http(sourceConfig.getPort());
+            if (sourceConfig.isSsl()) {


I think we should stick with isSsl. Making initials lowercase is a common convention. For example, java.net.http.HttpRequest.

dlvenable · 2021-10-05T20:38:51Z

data-prepper-plugins/http-source/build.gradle

    implementation "com.linecorp.armeria:armeria:1.9.2"
+    implementation "commons-io:commons-io:2.11.0"


Right now we have a versionsMap in our build-resources.gradle to provide this consistency.

Gradle 7 should help us improve this, but it is not possible to update presently due to the OpenSearch plugin.

dlvenable · 2021-10-05T20:40:15Z

...source/src/test/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceConfigTest.java

@@ -60,6 +69,38 @@ public void testValidConfig() {
        assertEquals(TEST_THREAD_COUNT, sourceConfig.getThreadCount());
        assertEquals(TEST_MAX_CONNECTION_COUNT, sourceConfig.getMaxConnectionCount());
        assertEquals(TEST_MAX_PENDING_REQUESTS, sourceConfig.getMaxPendingRequests());
+        assertFalse(sourceConfig.isSsl());


We are going to start using Hamcrest consistently for assertions. But, this is fine for now.

dlvenable · 2021-10-05T20:41:27Z

data-prepper-plugins/opensearch/build.gradle

        force 'commons-logging:commons-logging:1.2'
        force 'org.apache.httpcomponents:httpclient:4.5.13'
+        force 'org.apache.httpcomponents:httpcore:4.4.13'
        force "org.hdrhistogram:HdrHistogram:2.1.12"
        force 'joda-time:joda-time:2.10.10'


This might be here because the OpenSearch build plugin brings in its own dependencies.

Signed-off-by: qchea <[email protected]>

laneholloway · 2021-10-06T13:59:53Z

...va/com/amazon/dataprepper/plugins/source/loghttp/certificate/CertificateProviderFactory.java

+    }
+
+    public CertificateProvider getCertificateProvider() {
+        // TODO: support more certificate providers


Make an issue for this, please.

laneholloway · 2021-10-06T14:01:35Z

...va/com/amazon/dataprepper/plugins/source/loghttp/certificate/CertificateProviderFactory.java

+ *  GitHub history for details.
+ */
+
+package com.amazon.dataprepper.plugins.source.loghttp.certificate;


Not a blocker for now, but do we want to centralize the SSL certificate logic so we don't have these CertificateProviderFactories all over the codebase?

The reason I did not centralize this class in the PR is b/c right now it takes the source plugin config as input arg. We will need to refactor out a common SSLConfig with all possible relevant parameters in order to do this centralization. The SSLConfig will be reused by the existing source plugins. I will reserve it for future iteration and open an issue.

Signed-off-by: qchea <[email protected]>

dlvenable · 2021-10-06T18:33:24Z

data-prepper-plugins/common/build.gradle

@@ -16,8 +16,15 @@ dependencies {
    api project(':data-prepper-api')
    implementation 'com.fasterxml.jackson.core:jackson-databind'
    implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml'
+    implementation "commons-io:commons-io:2.11.0"


Nothing you have to fix now, but going forward, please use single quotes for Gradle strings. Double quotes are GStrings which are used for string interpolation.

…arch-project#359) * ENH: ssl config parameters and tests Signed-off-by: qchea <[email protected]> * MAINT: comment string Signed-off-by: qchea <[email protected]> * REF: server SSLCertProvider into common plugin Signed-off-by: qchea <[email protected]> * TST: ssl enabled server Signed-off-by: qchea <[email protected]> * MAINT: TODO comment Signed-off-by: qchea <[email protected]> * MAINT: update README Signed-off-by: qchea <[email protected]> * STY: rename ssl related parameters and variables Signed-off-by: qchea <[email protected]> * ENH: ssl config parameters and tests Signed-off-by: qchea <[email protected]> * MAINT: comment string Signed-off-by: qchea <[email protected]> * REF: server SSLCertProvider into common plugin Signed-off-by: qchea <[email protected]> * TST: ssl enabled server Signed-off-by: qchea <[email protected]> * MAINT: TODO comment Signed-off-by: qchea <[email protected]> * MAINT: update README Signed-off-by: qchea <[email protected]> * STY: rename ssl related parameters and variables Signed-off-by: qchea <[email protected]>

chenqi0805 added 6 commits September 30, 2021 17:46

ENH: ssl config parameters and tests

956caef

Signed-off-by: qchea <[email protected]>

MAINT: comment string

5756f6f

Signed-off-by: qchea <[email protected]>

REF: server SSLCertProvider into common plugin

35aa770

Signed-off-by: qchea <[email protected]>

TST: ssl enabled server

f6f799c

Signed-off-by: qchea <[email protected]>

MAINT: TODO comment

e947f7f

Signed-off-by: qchea <[email protected]>

MAINT: update README

763be20

Signed-off-by: qchea <[email protected]>

chenqi0805 requested review from dlvenable, cmanning09 and laneholloway October 5, 2021 16:24

laneholloway previously approved these changes Oct 5, 2021

View reviewed changes

dlvenable requested changes Oct 5, 2021

View reviewed changes

STY: rename ssl related parameters and variables

d496795

Signed-off-by: qchea <[email protected]>

chenqi0805 dismissed laneholloway’s stale review via d496795 October 5, 2021 22:55

chenqi0805 requested review from dlvenable and laneholloway October 5, 2021 23:04

laneholloway previously approved these changes Oct 6, 2021

View reviewed changes

chenqi0805 added 8 commits October 6, 2021 13:08

ENH: ssl config parameters and tests

d54fcf7

Signed-off-by: qchea <[email protected]>

MAINT: comment string

40a9181

Signed-off-by: qchea <[email protected]>

REF: server SSLCertProvider into common plugin

100d657

Signed-off-by: qchea <[email protected]>

TST: ssl enabled server

0583fe9

Signed-off-by: qchea <[email protected]>

MAINT: TODO comment

103bd01

Signed-off-by: qchea <[email protected]>

MAINT: update README

8611134

Signed-off-by: qchea <[email protected]>

STY: rename ssl related parameters and variables

bbcf96e

Signed-off-by: qchea <[email protected]>

MAINT: merge current main

151ca19

Signed-off-by: qchea <[email protected]>

chenqi0805 dismissed laneholloway’s stale review via 151ca19 October 6, 2021 18:19

dlvenable approved these changes Oct 6, 2021

View reviewed changes

dlvenable reviewed Oct 6, 2021

View reviewed changes

chenqi0805 merged commit ba0d9f1 into opensearch-project:main Oct 6, 2021

chenqi0805 deleted the enh/allow-tls branch October 6, 2021 18:37

chenqi0805 mentioned this pull request Oct 12, 2021

Enable TLS/SSL for http source plugin #358

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: enable TLS/SSL through local filepath in http source #359

ENH: enable TLS/SSL through local filepath in http source #359

chenqi0805 commented Oct 5, 2021 •

edited

Loading

laneholloway left a comment

laneholloway Oct 5, 2021

dlvenable Oct 5, 2021

chenqi0805 Oct 5, 2021

laneholloway Oct 6, 2021

laneholloway Oct 5, 2021

chenqi0805 Oct 5, 2021

dlvenable Oct 5, 2021

laneholloway Oct 5, 2021

dlvenable Oct 5, 2021

laneholloway Oct 6, 2021

dlvenable Oct 5, 2021

dlvenable Oct 5, 2021

dlvenable Oct 5, 2021

dlvenable Oct 5, 2021

laneholloway Oct 6, 2021

chenqi0805 Oct 6, 2021

laneholloway Oct 6, 2021

chenqi0805 Oct 6, 2021

chenqi0805 Oct 6, 2021

dlvenable Oct 6, 2021

		implementation "com.linecorp.armeria:armeria:1.9.2"
		implementation "commons-io:commons-io:2.11.0"

ENH: enable TLS/SSL through local filepath in http source #359

ENH: enable TLS/SSL through local filepath in http source #359

Conversation

chenqi0805 commented Oct 5, 2021 • edited Loading

Description

Issues Resolved

Check List

laneholloway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenqi0805 commented Oct 5, 2021 •

edited

Loading